Repository navigation
Round-2 audit fixes: strict v2 position_id (A-m5), physical-size read errors (A-m6), exact unit conversion (B-m7) - #142
Conversation
…it B-m7) to_micrometers multiplied by a binary approximation of the unit's factor, so 9 nm became 0.009000000000000001 µm while 0.009 µm stayed 0.009, and an exact square-pixel test called one size given in nm and in µm non-square. A unit below the micrometre now divides by its integer inverse factor and a larger one multiplies by its integer factor: one correctly rounded step, so a value exact in binary gives the double nearest its exact length in every unit. Tests check the operation for every unit over 40,000 values, that one length gives one double in every unit where it is exact, and that X in nm with Y in µm of the same size is not non_square_pixels. The CHANGELOG notes that sizes in µm from pm, Å or nm may change by 1 ULP. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G3qp9AxjpGPw9xfWAUJNqc
…dit A-m5) The v2 reader accepted a file entry without position_id or with null, and RunInputs then keyed that position by ""; an edited position_id was read as given. Version 2 now requires it and refuses any value other than position_id(relative_path) (the name without its extension, D85), naming the entry and the identifier its name gives; the writer refuses the same, so a written manifest always opens. RunInputs no longer substitutes "". The checks live in the new pure module manifest_fields (manifest.py was at 598 lines), with the roles check moved there. A manifest written by v0.4.0rc4 (tests/data/rc4_run_manifest.json, from make_rc4_manifest.py run with the rc4 code) is read in the tests; v1 manifests are read as before. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G3qp9AxjpGPw9xfWAUJNqc
test_run's position test deleted position_id from a v2 manifest and expected every row reused; that manifest is now refused, so the run reads every image again. The table case stays in test_run; the manifest case moves to test_manifest_position_id. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G3qp9AxjpGPw9xfWAUJNqc
…-m6) read_physical_sizes caught every exception and logged it at debug level, so a file it could not read, or a programming error, read as uncalibrated. Verified: a missing, truncated or non-TIFF file gave all-None sizes with no reason, and a resolution tag tifffile drops (value beyond EOF, unknown type; it only logs) left X silently uncalibrated next to a calibrated Y. Now opening errors (TIFF_OPEN_ERRORS: OSError, ValueError incl. TiffFileError, and the TypeError and struct.error tifffile raises on damaged IFDs, found by fuzzing) and metadata read errors (METADATA_READ_ERRORS: OSError, struct.error, TiffFileError) give no sizes with the reason in PhysicalSizes.invalid; a lone resolution tag names the lost one; any other exception is raised; a non-path is a TypeError. No metadata stays unknown. The value helpers move to the new module tiff_size_values (physical_sizes.py was at 600 lines), with full annotations: 5 fewer noqa ANN in the baseline. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G3qp9AxjpGPw9xfWAUJNqc
… is one double (B-m7) Dividing by the integer factor still left about a fifth of nm decimals 1 ULP from the same length written in µm (108.37 / 1000 is 0.10837000000000001). to_micrometers now moves the point of repr(value) by the unit's power of ten and rounds once with float: no binary factor and no decimal context. For a decimal of at most 15 significant digits this is the decimal written, so the same length in pm, Å, nm, µm, mm, cm or m gives the double float gives for it in µm. Overflow and underflow are still refused. Tests: exhaustive 4-digit decimals in every unit, seeded 1-15 digit decimals, the 16/17-digit limit, seeded doubles over the whole range against an exact Decimal oracle (deterministic, monotone, refused beyond the range), decimal-context independence. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G3qp9AxjpGPw9xfWAUJNqc
…docstring A position_id differing only in case or whitespace from its name was not tested (a casefold or strip comparison survived); the RunInputs guard, the documented open-error set and the unit gate of a lone resolution tag had no test. The moved _imagej_text docstring had its µ escape turned into a literal µ. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G3qp9AxjpGPw9xfWAUJNqc
|
Independent review (head e4b73b6, then 47334dd and 28bed66 pushed by the reviewer). A-m5: approved. Every writer since v2 (d9b109a, 1 Nov) records A-m6: approved. I fuzzed about 6,800 truncated or byte-flipped ImageJ and OME TIFFs: no exception escaped B-m7: changed to the shortest-decimal conversion.
"The same length in any unit gives the same double" holds for decimals of up to 15 significant digits. The claimed 17 is wrong: at 16 or 17 digits, two decimals can parse to the same double. Example: Tests. I strengthened tests where mutants survived: case-only or whitespace-only Gate:
Generated by Claude Code |
Q66 confirmed by the owner (3 November 2026); Q73's follow-up decided: neither a stated reshape nor a reorder in v0.4, kept in issue #144; Q74 records the tie-break and the consumers checked. The changelog gains a section for rc5 (PRs #139, #140, #142, #143) with migration notes: mounts refused, position_id required in v2, micrometre conversion within 1 ULP, --verify-content, and the tie-break. The migration guide gets the same. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G3qp9AxjpGPw9xfWAUJNqc
Three fixes from the cross-brick round-2 audit (3 November 2026). An independent review follows, so please do not merge yet.
Summary (by module)
A-m5:
position_idis required in v2 manifestsmanifest_fields.py(pure). It holdsrecorded_position_id(value, relative_path, strict=)and the roles check, which moved here frommanifest.py._file_entry) and the writer (_file_dict) refuse three cases, and each message names the entry and the identifier its name gives:position_id;position_id(relative_path)(D85: the image name without its extension, never parsed).run_inputs._fovno longer turns a missing id into"". It raisesRunInputsErrorinstead.manifest.pygoes from 598 to 592 lines.tests/data/rc4_run_manifest.jsonwas written by the v0.4.0rc4 code (git archive v0.4.0rc4, run throughtests/data/make_rc4_manifest.py). The test reads it. Its names cover.tif,.TIFF,.ome.tif, a name with dots, a space and.ome.zarr.position_idwas always filled by the writers: every rc tag (rc1 to rc4) sets it throughfile_entry_with, andimage_names.pyhas not changed since rc1.A-m6: TIFF calibration read errors (verified first)
XResolutiontag (value offset beyond the end of the file, or an unknown type), it drops the tag and only logs. X then read as uncalibrated with no reason, while Y stayed 0.5.tiff_size_values.py(io) with two documented exception sets:TIFF_OPEN_ERRORS = (OSError, ValueError, TypeError, struct.error)guards only thetifffile.TiffFile(path)call. Fuzzing tifffile 2026.10.15 showed that it raisesTypeErrorandstruct.erroron some damaged IFDs, as well asTiffFileError.METADATA_READ_ERRORS = (OSError, struct.error, TiffFileError)covers reading the metadata of an open file.PhysicalSizes.invalid, which produces theinvalid_physical_calibrationwarning.TypeError, because tifffile raisesValueErrorfor one.noqa: ANN).physical_sizes.pygoes from 600 to 566 lines, andphysical_sizes.read_physical_sizesis still importable (MorphoRelate imports it).B-m7: exact unit conversion
length_units._MICROMETERSis now a table of(multiplier, divisor)integers.9 nmnow gives0.009; before it gave0.009000000000000001.non_square_pixelscheck uses exact equality too, so X in nm with Y in µm for the same size no longer warns. There is a test for it.Decisions
inspect_tiffonly for tag-read errors. These now become theinvalid_physical_calibrationwarning instead of an unreadable file. A file that cannot be opened is stillunreadable_tiffin the folder check.to_micrometers(x, "nm") == x / 1000for every value tested;"0.009").Notes for review
0.1nm or108.33nm) can still differ by 1 ULP from the same length written in µm. In a seeded sample of decimals with up to 6 places, this happened for about 21% of nm values and 11% of mm values.repr. That would break the requested== x/1000property, so I did not do it.same_pixel_size) is not affected. Exact-equality tests in other bricks are affected only for such values.test_run.py: a test deletedposition_idfrom a v2 manifest and expected every row to be reused. That manifest is now refused (every image is read again). The table case stays intest_run.py; the manifest case moved totest_manifest_position_id.py.test_manifest.pynow fillsposition_id.test_audit_run_inputs.pynow expects/ 1000.tools/code_rules.pyis unchanged (SHA prefix cc04a420). The baseline only shrank:noqa ANNfrom 51 to 46,tests/test_run.pyfrom 862 to 860 lines.noqa.ruff check --ignore-noqapasses on every new file.Tests
ruff check .: all checks passed.ruff format --check .: 221 files already formatted.python tools/check.py --rules: passed.pytestwith.[dev,notebook,zarr](Python 3.13, Linux): 2138 passed, 4 skipped. The skips are expected: 3 case-sensitive file system checks and 1 file-permission check, because the tests run as root.test_exact_unit_conversion.py(11)test_manifest_position_id.py(15)test_tiff_size_read_errors.py(19)🤖 Generated with Claude Code
https://claude.ai/code/session_01G3qp9AxjpGPw9xfWAUJNqc